Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve proptypes oneof warnings - Fixes #1919 #6940

Conversation

philipshurpik
Copy link

Fixes issue #1919

As I see there is two problems:

  1. If oneOf uses function in array - so they will be stringified as null - so I add functionPrettifier method as argument to JSON.stringify
  2. Possibly mismatch oneOf and oneOfType - if checks do not pass and one of the function is one of React.PropTypes - proper warning will be shown

Welcome to your suggestions

reactEurope-hackathon

@philipshurpik philipshurpik changed the title Improve proptypes oneof warnings Improve proptypes oneof warnings - Fixes #1919 Jun 1, 2016
@chicoxyzzy
Copy link
Contributor

Could you provide a test case?

for (var i = 0; i < expectedValues.length; i++) {
if (is(propValue, expectedValues[i])) {
return null;
}
if (getPropType(expectedValues[i]) === 'function' && expectedValues[i].name === 'bound checkType') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you set a flag on the bound checkType instead? This way if we change its name, this won’t start creating false positives.

@gaearon
Copy link
Collaborator

gaearon commented Jun 26, 2016

Thanks for the PR! Let’s avoid using name and also this definitely needs some tests.

@philipshurpik
Copy link
Author

thanks, will work on improvements :)

@gaearon
Copy link
Collaborator

gaearon commented Oct 23, 2016

Closing in favor of #7526 but thanks for taking the time!

@gaearon gaearon closed this Oct 23, 2016
@aweary aweary mentioned this pull request Nov 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants